Skip to content

Update dev-environment-setup.ps1 #16278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Update dev-environment-setup.ps1 #16278

wants to merge 1 commit into from
+23 −10

Conversation

@elgatov
Copy link
Contributor

@elgatov elgatov commented Aug 31, 2025 •
edited
Loading

User description

💥 What does this PR do?

This PR fixes the parameters passed to the choco command. Because of the way powershell sends parameters to a command when it is external (MicrosoftDocs/PowerShell-Docs#2361), the params parameters passed where not being correctly interpreted by choco and would therefore not be taken into account. Now we use Start-Process with -ArgumentList which correctly parses and sends the parameters to choco.exe which allow us a couple of things:

  1. msys2 will be installed to the InstallDir (it just so happens that we use the default folder for installation)
  2. We can now add workloads to the VS 2022 Community installation, which has a couple of benefits:
  • We can add Microsoft.VisualStudio.Workload.NativeDesktop;includeRecommended and Microsoft.VisualStudio.Workload.ManagedDesktop which are necessary for compiling Selenium in Visual Studio
  • Now the installation of VS is completely automatic and does not need feedback from the user

🔄 Types of changes

  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Bug fix, Enhancement

Description

  • Fix PowerShell parameter passing to choco command using Start-Process

  • Automate Visual Studio 2022 workload installation without user interaction

  • Improve parameter handling with proper null checks and array construction

  • Remove manual Visual Studio setup prompts for fully automated installation

Diagram Walkthrough

flowchart LR
  A["PowerShell Script"] --> B["Start-Process with ArgumentList"]
  B --> C["Chocolatey Package Manager"]
  C --> D["Automated VS 2022 Installation"]
  D --> E["Native & Managed Desktop Workloads"]
Loading

File Walkthrough
Relevant files
Bug fix
dev-environment-setup.ps1
Fix choco parameter passing and automate VS installation 

scripts/dev-environment-setup.ps1

  • Replace direct choco calls with Start-Process and -ArgumentList
  • Add proper parameter array construction with null checks
  • Automate Visual Studio 2022 workload installation with specific
    components
  • Remove manual user prompts for Visual Studio setup
 +23/-10 

@elgatov
Update dev-environment-setup.ps1
f8bb9ef 
This PR fixes the parameters passed to the `choco` command. Because of the way powershell sends parameters to a command when it is external (MicrosoftDocs/PowerShell-Docs#2361), the `params` parameters passed where not being correctly interpreted by `choco` and would therefore not being taken into account. Now, we use `Start-Process` with `-ArgumentList` which correctly parses and sends the parameters to `choco.exe` which allow us a couple of things:

1. `msys2` will be installed to the `InstallDir´ (it just so happens that we use the default folder for installation)
2. We can not add workloads to the VS 2022 Community installation, which has a couple of benefits:
2.1. We can add `Microsoft.VisualStudio.Workload.NativeDesktop;includeRecommended` and `Microsoft.VisualStudio.Workload.ManagedDesktop` which are necessary for compiling Selenium in Visual Studio
2.2. Now the installation of VS is completely automatic and does not need feedback from the user
@selenium-ci selenium-ci added the B-build Includes scripting, bazel and CI integrations label Aug 31, 2025
@qodo-merge-pro
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

2361 - Partially compliant

Compliant requirements:

  • Fix invocation so external tools receive arguments exactly as intended.

Non-compliant requirements:

  • Ensure PowerShell parameter passing to external commands does not reuse or leak sessions/contexts inadvertently.
  • Validate isolation/independence of executions across runs.

Requires further human verification:

  • Run end-to-end environment setup multiple times to confirm no process/session reuse and consistent isolated installs.
  • Manually verify Chocolatey installs respect provided params (MSYS2 InstallDir, VS workloads) on clean and pre-provisioned machines.
  • Confirm VS components are installed without user prompts on CI images and developer machines.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

Passing $AdditionalParams as a single string into the $params array may result in it being treated as one argument; if it contains spaces, Start-Process may not split as intended. Consider building -ArgumentList as a flat string or an array of discrete tokens (e.g., split params for choco) and ensure proper quoting of --params value.

 
  $params = @(
    "install",
    $PackageName,
    "-y"
)

# Add AdditionalParams only if it has a value
if (-not [string]::IsNullOrWhiteSpace($AdditionalParams)) {
    $params += $AdditionalParams
}
Reliability

Using Get-Command to detect executables like 'devenv' and 'idea64' may fail due to PATH not updated in current session; consider verifying install via package presence or specific install paths, or refresh environment reliably before detection.

 
Write-Host "Checking installation of $PackageName"
if (-Not (Get-Command $ExecutableName -ErrorAction SilentlyContinue)) {
  Write-Host "Installing $PackageName..."
  #choco install $PackageName -y $AdditionalParams
  #Start-Process choco -ArgumentList 'install', 'visualstudio2022community', '--params "--add Microsoft.VisualStudio.Workload.NativeDesktop" --passive', '-y'
  Start-Process choco -ArgumentList $params -Verbose -NoNewWindow -Wait 
  refreshenv -Path ...
} else {
  Write-Host "$PackageName is already installed."
}
Quoting/Args

The choco --params strings include nested quotes; ensure correct escaping for PowerShell and choco (e.g., '--params="/InstallDir=C:\tools\msys64"' and workloads string). Cross-shell quoting differences may break on older PowerShell. Add a test run or build the string programmatically.

 
Install-ChocoPackage -PackageName "git" -ExecutableName "git" -AdditionalParams ""
Install-ChocoPackage -PackageName "bazelisk" -ExecutableName "bazel"  -AdditionalParams ""
Install-ChocoPackage -PackageName "msys2" -ExecutableName "C:\tools\msys64\usr\bin\bash.exe" -AdditionalParams '--params="/InstallDir=C:\tools\msys64"'
Update-EnvironmentVariables -VariableName "PATH" -Value "C:\tools\msys64\usr\bin"
Update-EnvironmentVariables -VariableName "BAZEL_SH" -Value "C:\tools\msys64\usr\bin\bash.exe"
Install-ChocoPackage -PackageName "visualstudio2022community" -ExecutableName "devenv" -AdditionalParams '--params="--add Microsoft.VisualStudio.Workload.NativeDesktop;includeRecommended --add Microsoft.VisualStudio.Workload.ManagedDesktop --passive"'

# Start-Process "C:\Program Files (x86)\Microsoft Visual Studio\Installer\setup.exe"

@qodo-merge-pro
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Fix Start-Process flow and refreshenv

The line "refreshenv -Path ..." is invalid PowerShell and will throw a parse
error, breaking the script; remove the placeholder or call refreshenv correctly.
Additionally, Start-Process does not propagate exit codes—use -PassThru and
check the ExitCode (or $LASTEXITCODE) to fail fast if choco installations fail.
This makes the automated setup reliable instead of silently continuing after
errors.

Examples:

scripts/dev-environment-setup.ps1 [29-30] 
    Start-Process choco -ArgumentList $params -Verbose -NoNewWindow -Wait 
    refreshenv -Path ...

Solution Walkthrough:

Before:

Function Install-ChocoPackage {
  param (
    [string]$PackageName,
    ...
  )
  ...
  if (-Not (Get-Command $ExecutableName ...)) {
    Write-Host "Installing $PackageName..."
    Start-Process choco -ArgumentList $params -Verbose -NoNewWindow -Wait 
    refreshenv -Path ...
  } else {
    Write-Host "$PackageName is already installed."
  }
}

After:

Function Install-ChocoPackage {
  param (
    [string]$PackageName,
    ...
  )
  ...
  if (-Not (Get-Command $ExecutableName ...)) {
    Write-Host "Installing $PackageName..."
    $process = Start-Process choco -ArgumentList $params -Wait -PassThru
    if ($process.ExitCode -ne 0) {
        throw "Choco installation of $PackageName failed with exit code $($process.ExitCode)"
    }
    refreshenv
  } else {
    Write-Host "$PackageName is already installed."
  }
}
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a fatal syntax error in refreshenv -Path ... and a critical reliability flaw by not checking the exit code from Start-Process, which is essential for a robust automation script.

High
Learned
best practice
Accept array for extra args

Make AdditionalParams a string[] and append it directly so callers can pass
multi-token arguments safely without brittle quoting.

scripts/dev-environment-setup.ps1 [7-22]

 param (
   [string]$PackageName,
   [string]$ExecutableName,
-  [string]$AdditionalParams = $null
+  [string[]]$AdditionalParams = @()
 )
 
 $params = @(
   "install",
   $PackageName,
   "-y"
 )
 
-# Add AdditionalParams only if it has a value
-if (-not [string]::IsNullOrWhiteSpace($AdditionalParams)) {
+if ($AdditionalParams -and $AdditionalParams.Count -gt 0) {
     $params += $AdditionalParams
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Use platform-appropriate argument passing; prefer typed arrays over concatenated strings to avoid quoting/splitting bugs.

Low
Check process exit codes

Capture the process and check the ExitCode to surface installation failures
instead of proceeding silently.

scripts/dev-environment-setup.ps1 [29]

-Start-Process choco -ArgumentList $params -Verbose -NoNewWindow -Wait
+$proc = Start-Process choco -ArgumentList $params -Verbose -NoNewWindow -Wait -PassThru
+if ($proc.ExitCode -ne 0) {
+  throw "Chocolatey install failed for $PackageName with exit code $($proc.ExitCode)."
+}
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Add defensive checks around external process execution; verify exit codes and fail fast on errors.

Low
General
Remove hardcoded debug comments

The commented debug line contains hardcoded package name
'visualstudio2022community' which doesn't match the dynamic $PackageName
parameter. This leftover debug code should be removed to avoid confusion.

scripts/dev-environment-setup.ps1 [27-29]

-#choco install $PackageName -y $AdditionalParams
-#Start-Process choco -ArgumentList 'install', 'visualstudio2022community', '--params "--add Microsoft.VisualStudio.Workload.NativeDesktop" --passive', '-y'
 Start-Process choco -ArgumentList $params -Verbose -NoNewWindow -Wait
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: This is a valid code cleanup suggestion that improves readability by removing commented-out debug code, which is good practice but has a low impact on functionality.

Low
  • More

@diemol diemol requested a review from nvborisenko September 1, 2025 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nvborisenko nvborisenko Awaiting requested review from nvborisenko

Assignees

No one assigned

Labels

B-build Includes scripting, bazel and CI integrations Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@elgatov @selenium-ci